Fix `/build` slash command to checkout the correct PR branch #2542

echoix · 2023-04-08T18:06:54Z

The /build slash command was not checking out the correct branch when used. It was always using the base branch of the upstream repo. This was because it was called from a workflow_dispatch, so not tied to any event, nor pull request. The dispatcher (another workflow run), only had access to an issue event context, that doesn’t include information from the repo of the pull request. So, in order to hand the checkout action appropriate inputs, the build command is passed additional inputs, for the ref and repo name. These are obtained by using an api call from the pull request number in the repo (since it is available), and extracting the fields from the json.

I tried using some pre-made actions, but didn’t get the repo name easily, and they weren’t mainstream. So, it is just using a custom script with actions/github-script to do the api call, and set outputs.

Documentation for the usage is the same, it should do exactly the same as expected.

Fixes # … I’m not sure we reported it officially, but we (maintainers) are aware that it didn’t work directly as wanted.

Proposed Changes

...
...
...

Readiness Checklist

Author/Contributor

Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

Label as breaking if this is a large fundamental change
Label as either automation, bug, documentation, enhancement, infrastructure, or performance

Add checkout-ref and checkout-repository

Add checkout-ref and checkout-repository inputs

Slash cmd ref

Slash cmd ref2

Slash cmd ref3

Update slash-command-dispatch.yml 4

Slash cmd ref

Update slash-command-dispatch.yml

Update slash-command-dispatch.yml 8

Update slash-command-dispatch.yml9

Revert "Dummy update descriptor"

Update slash-command-dispatch.yml

Change Python version

Update slash-cmd-ref from oxsecurity/megalinter main

echoix · 2023-04-08T18:11:42Z

🦙 MegaLinter status: ⚠️ WARNING

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ BASH	bash-exec	6		0	0.01s
✅ BASH	shellcheck	6		0	0.14s
✅ BASH	shfmt	6	0	0	0.42s
✅ COPYPASTE	jscpd	yes		no	2.83s
✅ DOCKERFILE	hadolint	116		0	19.72s
✅ JSON	eslint-plugin-jsonc	21	0	0	2.44s
✅ JSON	jsonlint	19		0	0.22s
✅ JSON	v8r	21		0	14.69s
⚠️ MARKDOWN	markdownlint	312	0	230	7.83s
✅ MARKDOWN	markdown-link-check	312		0	6.22s
✅ MARKDOWN	markdown-table-formatter	312	0	0	24.1s
✅ OPENAPI	spectral	1		0	1.51s
⚠️ PYTHON	bandit	185		54	2.25s
✅ PYTHON	black	185	0	0	5.17s
✅ PYTHON	flake8	185		0	1.88s
✅ PYTHON	isort	185	0	0	0.88s
✅ PYTHON	mypy	185		0	8.07s
✅ PYTHON	pylint	185		0	12.66s
⚠️ PYTHON	pyright	185		251	17.96s
✅ PYTHON	ruff	185	0	0	0.47s
✅ REPOSITORY	checkov	yes		no	37.87s
✅ REPOSITORY	git_diff	yes		no	0.4s
✅ REPOSITORY	secretlint	yes		no	15.93s
✅ REPOSITORY	trivy	yes		no	34.77s
✅ SPELL	cspell	753		0	23.19s
✅ SPELL	misspell	572	0	0	1.07s
✅ XML	xmllint	3	0	0	0.42s
✅ YAML	prettier	81	0	0	3.21s
✅ YAML	v8r	23		0	61.35s
✅ YAML	yamllint	82		0	1.3s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by

echoix · 2023-04-08T18:12:39Z

Limitations I forgot/couldn’t easily test yet:

Check that when used from another repo, with another unrelated user, the pulled branch (from the PR) can be pushed (has write access). Obviously the user making the PR should have checked the box to allow maintainers to update the branch. You see why I couldn’t test it easily by myself, with my PRs and my fork.
Check that the ref=… argument still works, or still works the same way as it is written in the output of the /help command, and the contribution guide. This I forgot to test

echoix · 2023-04-08T18:13:46Z

Please squash-merge rather than merge this PR. I had to make a lot of commits (and merge what changing the dispatcher to test) since it was Actions-related work

Kurt-von-Laven · 2023-04-09T01:54:58Z

I think as long as the proper permissions block is specified, the first limitation will be/already is addressed. When someone forks a repo, those permissions being specified in the workflow keeps them the same between repositories even when some users have chosen to default them to read-all instead of write-all. I believe organization admins can limit those permissions, but there's nothing further we can do about that besides ask people to reopen the PR from a personal fork without such restrictions.

echoix · 2023-04-09T02:53:09Z

That's not really what I meant actually. It's that when testing, the final state that I'm adding is like my 4th or 5th approach, and one of the failed attempts that was working was using a ref of refs/pull/${{ github.event.inputs.issue-number }}/head , but finally it is read-only, it couldn't be pushed to. So I just couldn't test that there isn't a remaining permission that would block in a same manner, that I couldn't have encountered yet.

Kurt-von-Laven · 2023-04-09T08:07:47Z

Was ${{ github.event.inputs.issue-number }} the PR number? Do I understand correctly that you are asking whether pushing to <branch name> rather than refs/pull/<PR number>/head will work when future PRs are opened by non-maintainers who have checked the box to allow edits by maintainers, and a maintainer comments /build on their PR while logged into GitHub? If so, in principle, the contributor might have pretty complex branch protection rules that could result in a permission error that isn't preventable by speciying the proper workflow permissions. There's nothing we can do about that besides ask them to relax the branch protection rule(s) though, and it seems like a rare scenario. All refs of the form refs/pull/<PR number>/head are read-only on GitHub in all circumstances, so you did the right thing by using the branch name as the ref instead.

echoix · 2023-04-09T10:37:20Z

Yes, that's what I'm not sure. And yes, the issue-number is the PR number. There is no distinction on that matter api-wise

Kurt-von-Laven · 2023-04-09T18:44:28Z

I edited my reply to basically say, in that case it sounds like you did the right thing.

echoix · 2023-04-11T22:44:12Z

I think it is ready to merge, there's nothing left to do.

nvuillam · 2023-04-12T19:15:38Z

@echoix except conflict it's ok :)

echoix and others added 30 commits April 4, 2023 19:10

Update slash-command-dispatch.yml

ac8953f

Add checkout-ref and checkout-repository

Update build-command.yml

74ac9da

Add checkout-ref and checkout-repository inputs

Merge pull request #4 from echoix/slash-cmd-ref

1e8bb1e

Slash cmd ref

Update slash-command-dispatch.yml

80450d6

Update build-command.yml

bb03758

Update build-command.yml

b5eee02

Merge pull request #6 from echoix/slash-cmd-ref

faf0166

Slash cmd ref2

Dummy update descriptor

fdd4762

Update slash-command-dispatch.yml

e1fc725

Update build-command.yml

2e7d286

Merge branch 'main' into slash-cmd-ref

5c4d9e4

Merge pull request #7 from echoix/slash-cmd-ref

9b868a6

Slash cmd ref3

[build-command] Update generated files

3f32807

Update slash-command-dispatch.yml

3d03323

Merge pull request #8 from echoix/slash-cmd-ref

ea8793f

Update slash-command-dispatch.yml 4

Merge branch 'main' into slash-cmd-ref-test

4e96161

Update slash-command-dispatch.yml

ec6dd3b

Update slash-command-dispatch.yml

6a00da2

Merge pull request #9 from echoix/slash-cmd-ref

44706cc

Slash cmd ref

Update slash-command-dispatch.yml

bc179b9

Merge pull request #10 from echoix/slash-cmd-ref

bf8674c

Update slash-command-dispatch.yml

Update slash-command-dispatch.yml

cd18df0

Merge pull request #11 from echoix/slash-cmd-ref

383a2ba

Update slash-command-dispatch.yml

Merge branch 'main' into slash-cmd-ref-test

a5c250b

Update slash-command-dispatch.yml

7f1a853

Merge pull request #12 from echoix/slash-cmd-ref

374b2e7

Update slash-command-dispatch.yml 8

Merge branch 'main' into slash-cmd-ref-test

9f9ce7e

Update slash-command-dispatch.yml

0ea9cd5

Merge pull request #13 from echoix/slash-cmd-ref

0635469

Update slash-command-dispatch.yml9

Merge branch 'main' into slash-cmd-ref-test

6a4b70c

echoix added 10 commits April 8, 2023 12:46

Revert "Dummy update descriptor"

6d47693

Merge pull request #18 from echoix/revert-5-slash-cmd-ref-test

9d5eb27

Revert "Dummy update descriptor"

Update slash-command-dispatch.yml

870adcc

Update slash-command-dispatch.yml

6b6f31a

Merge branch 'main' into slash-cmd-ref

4df0673

Merge pull request #20 from echoix/slash-cmd-ref

8b87cb9

Update slash-command-dispatch.yml

Update build-command.yml

820aa7a

Change Python version

Update slash-command-dispatch.yml

cd2a427

Merge branch 'main' into slash-cmd-ref

e6c60de

Merge pull request #22 from oxsecurity/main

9ce1b41

Update slash-cmd-ref from oxsecurity/megalinter main

echoix added github_actions Pull requests that update Github_actions code automation labels Apr 8, 2023

echoix self-assigned this Apr 8, 2023

echoix requested a review from nvuillam as a code owner April 8, 2023 18:06

Update CHANGELOG.md

76cc69b

nvuillam approved these changes Apr 12, 2023

View reviewed changes

Merge branch 'main' into slash-cmd-ref

bfd950b

bdovaz enabled auto-merge (squash) April 12, 2023 19:21

nvuillam disabled auto-merge April 12, 2023 19:22

nvuillam merged commit cdec507 into oxsecurity:main Apr 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `/build` slash command to checkout the correct PR branch #2542

Fix `/build` slash command to checkout the correct PR branch #2542

echoix commented Apr 8, 2023 •

edited

Loading

echoix commented Apr 8, 2023

echoix commented Apr 8, 2023

echoix commented Apr 8, 2023 •

edited

Loading

Kurt-von-Laven commented Apr 9, 2023 •

edited

Loading

echoix commented Apr 9, 2023

Kurt-von-Laven commented Apr 9, 2023 •

edited

Loading

echoix commented Apr 9, 2023

Kurt-von-Laven commented Apr 9, 2023

echoix commented Apr 11, 2023

nvuillam commented Apr 12, 2023

Fix /build slash command to checkout the correct PR branch #2542

Fix /build slash command to checkout the correct PR branch #2542

Conversation

echoix commented Apr 8, 2023 • edited Loading

Proposed Changes

Readiness Checklist

Author/Contributor

Reviewing Maintainer

echoix commented Apr 8, 2023

🦙 MegaLinter status: ⚠️ WARNING

echoix commented Apr 8, 2023

echoix commented Apr 8, 2023 • edited Loading

Kurt-von-Laven commented Apr 9, 2023 • edited Loading

echoix commented Apr 9, 2023

Kurt-von-Laven commented Apr 9, 2023 • edited Loading

echoix commented Apr 9, 2023

Kurt-von-Laven commented Apr 9, 2023

echoix commented Apr 11, 2023

nvuillam commented Apr 12, 2023

Fix `/build` slash command to checkout the correct PR branch #2542

Fix `/build` slash command to checkout the correct PR branch #2542

echoix commented Apr 8, 2023 •

edited

Loading

echoix commented Apr 8, 2023 •

edited

Loading

Kurt-von-Laven commented Apr 9, 2023 •

edited

Loading

Kurt-von-Laven commented Apr 9, 2023 •

edited

Loading